ADFA-2594: Fix plugin fragment crash on configuration change#853
Conversation
When UI settings (font size, dark mode) change, Android recreates the
Activity and FragmentManager tries to restore plugin fragments using
the default classloader, which doesn't have access to plugin DEX classes.
- Add PluginFragmentFactory to instantiate plugin fragments using the
correct DexClassLoader during fragment restoration
- Register plugin classloaders when fragments are first created
- Save/restore open plugin tab IDs via SharedPreferences to persist
tab UI across configuration changes
📝 WalkthroughWalkthroughAdds persistent plugin-tab state and a plugin-aware FragmentFactory. Plugin class loaders are exposed and registered so plugin fragments can be instantiated via their plugin ClassLoaders; activity, adapter, and tab manager are wired to save/restore and register/unregister plugin fragment class loaders. Changes
Sequence DiagramssequenceDiagram
participant Activity as EditorHandlerActivity
participant FragMgr as FragmentManager
participant PluginFF as PluginFragmentFactory
participant Adapter as EditorBottomSheetTabAdapter
participant TabMgr as PluginEditorTabManager
participant PluginMgr as PluginManager
Activity->>Activity: onCreate()
Activity->>PluginFF: setupPluginFragmentFactory() (install factory)
Activity->>Activity: restoreOpenedPluginTabs()
Adapter->>PluginMgr: getClassLoaderForPlugin(plugin)
PluginMgr-->>Adapter: ClassLoader?
Adapter->>PluginFF: registerPluginClassLoader(pluginId, classLoader, [className])
TabMgr->>PluginMgr: loadPluginTabs() (store pluginManagerRef)
TabMgr->>TabMgr: getOrCreateTabFragment(extension)
TabMgr->>PluginMgr: getClassLoaderForPlugin(extension.plugin)
PluginMgr-->>TabMgr: ClassLoader?
TabMgr->>PluginFF: registerPluginClassLoader(pluginId, classLoader, [className])
TabMgr->>FragMgr: request instantiate(className)
FragMgr->>PluginFF: instantiate(classLoader, className)
PluginFF->>PluginFF: hasClassLoaderForFragment(className)?
alt Registered
PluginFF->>PluginFF: load class via plugin ClassLoader & construct
PluginFF-->>FragMgr: return plugin Fragment
else Unregistered
PluginFF->>FragMgr: delegate to default factory
end
Activity->>Activity: onPause()
Activity->>Activity: saveOpenedPluginTabs()
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested Reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
`@plugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/fragment/PluginFragmentFactory.kt`:
- Around line 14-46: The shared mutable map pluginClassLoaders is not
thread-safe; replace it with a thread-safe implementation (e.g., a
java.util.concurrent.ConcurrentHashMap) or wrap it with
Collections.synchronizedMap and update its declaration accordingly so all
accesses in registerPluginClassLoader, unregisterPluginClassLoader,
hasClassLoaderForFragment, getClassLoaderForFragment, and clearAllClassLoaders
are safe; ensure you do not rely on non-atomic compound operations (if you do,
add explicit synchronization around those operations).
- Around line 24-30: Call PluginFragmentFactory.unregisterPluginClassLoader(...)
from PluginManager.unloadPlugin() to remove classloaders for that plugin and
avoid leaks: inside PluginManager.unloadPlugin() (after or alongside the
existing PluginFragmentHelper.unregisterPluginContext(...) call), gather the
list of fragment class names associated with the plugin (e.g., from the plugin's
metadata/manifest or stored registration used by registerPluginClassLoader
calls) and invoke PluginFragmentFactory.unregisterPluginClassLoader(pluginId,
fragmentClassNames) so entries are removed from the pluginClassLoaders map;
ensure you pass the exact fragment class name list that was registered when the
plugin was loaded.
🧹 Nitpick comments (4)
plugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/ui/PluginEditorTabManager.kt (2)
32-53: Consider using direct singleton access instead of caching the reference.Storing
pluginManagerRefcreates a dependency on the caller remembering to callloadPluginTabs()beforegetOrCreateTabFragment(). While the current code handles this with a warning, consider accessingPluginManager.getInstance()directly inregisterFragmentClassLoaderfor consistency with other similar code inEditorBottomSheetTabAdapter.♻️ Suggested approach
- private var pluginManagerRef: PluginManager? = null ... private fun registerFragmentClassLoader(extension: EditorTabExtension, fragment: Fragment) { - val pluginManager = pluginManagerRef ?: run { + val pluginManager = PluginManager.getInstance() ?: run { logger.warn("PluginManager not available, cannot register fragment classloader") return }
137-156: Consider using debug level for successful registration logs.The
logger.infoon line 152 will produce logs for every plugin fragment creation. For production,logger.debugwould be more appropriate to avoid log noise, consistent with the debug-level logging used inEditorBottomSheetTabAdapter.registerPluginFragmentClassLoader.♻️ Suggested change
- logger.info("Registered classloader for fragment {} from plugin {}", fragmentClassName, pluginId) + logger.debug("Registered classloader for fragment {} from plugin {}", fragmentClassName, pluginId)app/src/main/java/com/itsaky/androidide/activities/editor/EditorHandlerActivity.kt (1)
219-229: Consider using the class logger for consistency.The method uses
Log.ddirectly while the rest of the class useslog(the SLF4J logger). For consistency and to benefit from any logging configuration, consider using the class logger.♻️ Suggested change
- Log.d("EditorHandlerActivity", "Saved open plugin tabs: $openPluginTabIds") + log.debug("Saved open plugin tabs: {}", openPluginTabIds)plugin-manager/src/main/kotlin/com/itsaky/androidide/plugins/manager/fragment/PluginFragmentFactory.kt (1)
49-67: Consider using Kotlin's reflection or updated Java API.
constructor.isAccessible = trueis deprecated. While it still works, consider usingconstructor.trySetAccessible()(Java 9+) or Kotlin'sisAccessibleproperty which handles the deprecation more gracefully.♻️ Suggested change
val fragmentClass = pluginClassLoader.loadClass(className) val constructor = fragmentClass.getDeclaredConstructor() - constructor.isAccessible = true + if (!constructor.trySetAccessible()) { + Log.w(TAG, "Could not make constructor accessible for: $className") + } constructor.newInstance() as FragmentOr simply rely on the fact that
newInstance()will throw if inaccessible, which your catch block already handles.
2 participants
When UI settings (font size, dark mode) change, Android recreates the
Activity and FragmentManager tries to restore plugin fragments using
the default classloader, which doesn't have access to plugin DEX classes.